Bundle the kind with the con by nomeata · Pull Request #163 · caffeinelabs/motoko

nomeata · 2019-02-08T17:28:17Z

The benefit is that many passes no longer need to keep a con_env
around, and many operations in Type are now always available, in
partiuclar Type.as_*_sub, promote etc.

The downside is that, because of the multi-pass approach in typing.ml, the
annotation needs to be a kind ref.

I tried using a promise, but it seems that certain things are run more
than once in typing (the same has caused issues with recoverable error
messages before). I am not sure if this is avoidable.

If maybe the initialization can be made a bit cleaner in typing, then
I think this will improve the life of those working on the Ir
noticably.

The benefit is that many passes no longer need to keep a `con_env` around, and many operations in `Type` are now always available, in partiuclar `Type.as_*_sub`, `promote` etc. The downside is that, because of the multi-pass approach in `typing.ml`, the annotation needs to be a `kind ref`. I tried using a `promise`, but it seems that certain things are run more than once in `typing` (the same has caused issues with recoverable error messages before). I am not sure if this is avoidable. If maybe the initialization can be made a bit cleaner in `typing`, then I think this will improve the life of those working on the `Ir` noticably.

the `kind` is already in the `con`, so no need having it here.

rossberg

Generally this seems like a workable direction to me, though there perhaps is potential for a simpler solution. In particular, it seems that one of two choices should be possible:

Instead of introducing the new Type.con, put the kind directly on typ's Con constructor. That makes clear that it is only a use-site annotation.
Or use Type.con throughout and eliminate con_env altogether. Then a con is annotated everywhere. In that case, I would probably change Con.t to 'a Con.t, such that the kind can become a proper part of the existing con.

Another way to put this is that it makes sense to have either Type.con or con_env, but having both seems redundant.

rossberg · 2019-02-08T17:44:38Z

src/type.ml

+(* the promise is only there to break the recursion in open_bind *)
+let kind con = !(con.kind)
+
+let fresh_con n = { con = Con.fresh n; kind = ref (Abs ([],Pre)) }


It would be cleaner to make the kind a parameter than to hardwire a Pre in here. Would save a few spurious assignments, too.

Stale comment mentions a promise...

Yes, this was a hold-over from when I tried using a promise. Which would be so much cleaner…

I think the problem is that infer_dec_typdecs is run multiple times, and there is no good time when we can do a single fulfill.

I have pushed a commit that makes the ref a bit promise-like, in that it only allows going from a Pre-type to a non-pre-type once, and then complains if something wants to set the kind a second time with a different non-pre-type. But I am not sure if it is worth the bother, and can easily revert d5baf2b.

src/type.ml

crusso

Looks good to me but I've had a few beers

src/desugar.ml

so that the choice whether it is a `promise`, or a `ref`, or something else, private to `type.ml`. I tried to make it more promise-like, by trapping when the kind is set a second time to something different, but it seems that `k = k'` can loop.

with a different value. A bit like a promise, but more compatible with `typing.ml` running `infer_dec_typdecs` multiple times. Not sure if it is worth the bother, though.

nomeata · 2019-02-09T10:34:54Z

Instead of introducing the new Type.con, put the kind directly on typ's Con constructor. That makes clear that it is only a use-site annotation.

This makes very much sense in terms of the types, but it is harder with the the delayed determination of kinds in open_binds and in typing. A ref is (unfortunately :-() more convenient for the late assignments. (One could put a ref in the occurrences, but this has issues again with passes that have to traverse all types, such as async).

Or use Type.con throughout and eliminate con_env altogether. Then a con is annotated everywhere. In that case, I would probably change Con.t to 'a Con.t, such that the kind can become a proper part of the existing con.

That is a reasonable refactoring over what we have right now. If you are happy with this change semantic-wise, I’ll do that refactoring.

Although: There is a function avoid that takes two con_env. What does it do? How would it work in a world without con_env? (It is the only function in type.mli that still takes a con_env.)

rossberg · 2019-02-11T11:26:09Z

Instead of introducing the new Type.con, put the kind directly on typ's Con constructor. That makes clear that it is only a use-site annotation.

This makes very much sense in terms of the types, but it is harder with the the delayed determination of kinds in open_binds and in typing. A ref is (unfortunately :-() more convenient for the late assignments. (One could put a ref in the occurrences, but this has issues again with passes that have to traverse all types, such as async).

Well, it would still be a ref, just elsewhere.

Or use Type.con throughout and eliminate con_env altogether. Then a con is annotated everywhere. In that case, I would probably change Con.t to 'a Con.t, such that the kind can become a proper part of the existing con.

That is a reasonable refactoring over what we have right now. If you are happy with this change semantic-wise, I’ll do that refactoring.

Although: There is a function avoid that takes two con_env. What does it do? How would it work in a world without con_env? (It is the only function in type.mli that still takes a con_env.)

Avoid tries to expand local type defs when they go out of scope. I think after this change it would have to work with a typ_env as input and denote the corresponding cons through that. (It really only needs a con set.)

Can you implement one of these before landing?

nomeata · 2019-02-11T11:34:03Z

Well, it would still be a ref, just elsewhere.

What I mean is: With a ref at the binding site, there is a single place to update the ref once. With the ref only at all occurences, then it is hard to avoid updating it multiple times (which is bad when the modification is not idempotent, or might not terminate with recursive definitions.

Avoid tries to expand local type defs when they go out of scope. I think after this change it would have to work with a typ_env as input and denote the corresponding cons through that. (It really only needs a con set.)

Ok, that is roughly what I expected, including that a con_set ought to be enough. Is this just a best-effort thing for better error messages?

BTW, this loops in avoid (reported in #170):

{ type Rec = ?Rec; (null : Rec) };

Can you implement one of these before landing?

Yes, sure (the second, though).

In `typing.ml`, we replace it with a `con_set`, so that we can still call `avoid`. In `check_ir.ml` we simply drop the field.

it is always the range of `typ_env` anyways. This simplifies a few more functions.

nomeata · 2019-02-11T14:30:29Z

Can you implement one of these before landing?

Refactoring done. It is unclear whether the fact that the annotation of a Con.t is mutable should be determined in con.ml or in type.ml.

which uncovers some dead code.

Changed have been made since Claudio’s review.

rossberg · 2019-02-13T17:19:04Z

Hm, you already got my review a couple of hours ago. ;)

nomeata · 2019-02-13T20:40:52Z

Hm, you already got my review a couple of hours ago. ;)

Eh, where? I am confused. Did you actually submit the review, or maybe just add the comments? In any case I am expecting GitHub to say “Andreas requested changes” or “Andreas approves the PR”.

src/type.ml

src/typing.ml

rossberg · 2019-02-14T09:06:16Z

Oops, sorry, I forgot to click the Submit button. (It's confusing that GH already shows the comments on the conversation page for me.)

src/typing.ml

src/type.ml

crusso · 2019-02-20T16:29:14Z

@rossberg shall I merge?

rossberg · 2019-02-20T16:30:13Z

Go for it!

nomeata · 2019-02-20T17:12:24Z

Whoohoo

matthewhammer · 2019-02-20T17:24:02Z

👏 👏

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...d4db8d07](dfinity/ic-hs@406decf...d4db8d0) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...31d535d2](dfinity/ic-hs@406decf...31d535d) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163)) * [`31d535d2`](dfinity/ic-hs@31d535d) increase the number of allowed delegations in a request from 4 to 20 ([dfinity/ic-hs⁠#166](https://github.com/dfinity/ic-hs/issues/166))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...4a310c0d](dfinity/ic-hs@406decf...4a310c0) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163)) * [`31d535d2`](dfinity/ic-hs@31d535d) increase the number of allowed delegations in a request from 4 to 20 ([dfinity/ic-hs⁠#166](https://github.com/dfinity/ic-hs/issues/166)) * [`4a310c0d`](dfinity/ic-hs@4a310c0) support ic0.is_controller ([dfinity/ic-hs⁠#169](https://github.com/dfinity/ic-hs/issues/169))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...5fc27bdc](dfinity/ic-hs@406decf...5fc27bd) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163)) * [`31d535d2`](dfinity/ic-hs@31d535d) increase the number of allowed delegations in a request from 4 to 20 ([dfinity/ic-hs⁠#166](https://github.com/dfinity/ic-hs/issues/166)) * [`4a310c0d`](dfinity/ic-hs@4a310c0) support ic0.is_controller ([dfinity/ic-hs⁠#169](https://github.com/dfinity/ic-hs/issues/169)) * [`5fc27bdc`](dfinity/ic-hs@5fc27bd) do not include keep-alive header in httpbin response ([dfinity/ic-hs⁠#170](https://github.com/dfinity/ic-hs/issues/170))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...96448083](dfinity/ic-hs@406decf...9644808) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163)) * [`31d535d2`](dfinity/ic-hs@31d535d) increase the number of allowed delegations in a request from 4 to 20 ([dfinity/ic-hs⁠#166](https://github.com/dfinity/ic-hs/issues/166)) * [`4a310c0d`](dfinity/ic-hs@4a310c0) support ic0.is_controller ([dfinity/ic-hs⁠#169](https://github.com/dfinity/ic-hs/issues/169)) * [`5fc27bdc`](dfinity/ic-hs@5fc27bd) do not include keep-alive header in httpbin response ([dfinity/ic-hs⁠#170](https://github.com/dfinity/ic-hs/issues/170)) * [`96448083`](dfinity/ic-hs@9644808) drop nix-build-uncached ([dfinity/ic-hs⁠#175](https://github.com/dfinity/ic-hs/issues/175))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...a1b3f670](dfinity/ic-hs@406decf...a1b3f67) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163)) * [`31d535d2`](dfinity/ic-hs@31d535d) increase the number of allowed delegations in a request from 4 to 20 ([dfinity/ic-hs⁠#166](https://github.com/dfinity/ic-hs/issues/166)) * [`4a310c0d`](dfinity/ic-hs@4a310c0) support ic0.is_controller ([dfinity/ic-hs⁠#169](https://github.com/dfinity/ic-hs/issues/169)) * [`5fc27bdc`](dfinity/ic-hs@5fc27bd) do not include keep-alive header in httpbin response ([dfinity/ic-hs⁠#170](https://github.com/dfinity/ic-hs/issues/170)) * [`96448083`](dfinity/ic-hs@9644808) drop nix-build-uncached ([dfinity/ic-hs⁠#175](https://github.com/dfinity/ic-hs/issues/175)) * [`c6fbe1f7`](dfinity/ic-hs@c6fbe1f) increase ingress_expiry in reference test suite ([dfinity/ic-hs⁠#176](https://github.com/dfinity/ic-hs/issues/176)) * [`a1b3f670`](dfinity/ic-hs@a1b3f67) add Connection: close header to httpbin ([dfinity/ic-hs⁠#178](https://github.com/dfinity/ic-hs/issues/178))

## Changelog for ic-hs: Branch: master Commits: [dfinity/ic-hs@406decfa...9152a0ff](dfinity/ic-hs@406decf...9152a0f) * [`d4db8d07`](dfinity/ic-hs@d4db8d0) bump nixpkgs to 7c786944f801745310578d1cfc019923396f830c ([dfinity/ic-hs⁠#163](https://github.com/dfinity/ic-hs/issues/163)) * [`31d535d2`](dfinity/ic-hs@31d535d) increase the number of allowed delegations in a request from 4 to 20 ([dfinity/ic-hs⁠#166](https://github.com/dfinity/ic-hs/issues/166)) * [`4a310c0d`](dfinity/ic-hs@4a310c0) support ic0.is_controller ([dfinity/ic-hs⁠#169](https://github.com/dfinity/ic-hs/issues/169)) * [`5fc27bdc`](dfinity/ic-hs@5fc27bd) do not include keep-alive header in httpbin response ([dfinity/ic-hs⁠#170](https://github.com/dfinity/ic-hs/issues/170)) * [`96448083`](dfinity/ic-hs@9644808) drop nix-build-uncached ([dfinity/ic-hs⁠#175](https://github.com/dfinity/ic-hs/issues/175)) * [`c6fbe1f7`](dfinity/ic-hs@c6fbe1f) increase ingress_expiry in reference test suite ([dfinity/ic-hs⁠#176](https://github.com/dfinity/ic-hs/issues/176)) * [`a1b3f670`](dfinity/ic-hs@a1b3f67) add Connection: close header to httpbin ([dfinity/ic-hs⁠#178](https://github.com/dfinity/ic-hs/issues/178)) * [`d3812ffc`](dfinity/ic-hs@d3812ff) increase delegation expiry in tests ([dfinity/ic-hs⁠#182](https://github.com/dfinity/ic-hs/issues/182)) * [`40a46e2f`](dfinity/ic-hs@40a46e2) sync universal-canister with IC repo ([dfinity/ic-hs⁠#177](https://github.com/dfinity/ic-hs/issues/177)) * [`7a6259c2`](dfinity/ic-hs@7a6259c) decrease number of threads and request submission latency ([dfinity/ic-hs⁠#179](https://github.com/dfinity/ic-hs/issues/179)) * [`a9f73dba`](dfinity/ic-hs@a9f73db) fix decoding compressed WASM modules during snapshotting ([dfinity/ic-hs⁠#184](https://github.com/dfinity/ic-hs/issues/184)) * [`64c19a95`](dfinity/ic-hs@64c19a9) bump nixpkgs to eaf03591711b46d21abc7082a8ebee4681f9dbeb ([dfinity/ic-hs⁠#189](https://github.com/dfinity/ic-hs/issues/189)) * [`9152a0ff`](dfinity/ic-hs@9152a0f) add date header to httpbin responses and make http header names lower-case ([dfinity/ic-hs⁠#188](https://github.com/dfinity/ic-hs/issues/188)) Includes and closes #3915. Reason: `ic-hs` and `nixpkgs` must be in sync, so that the artefact caching can work.

nomeata added 2 commits February 8, 2019 18:24

Simplify Ir.TypD

0dde544

the `kind` is already in the `con`, so no need having it here.

rossberg reviewed Feb 8, 2019

View reviewed changes

crusso previously approved these changes Feb 8, 2019

View reviewed changes

src/desugar.ml Show resolved Hide resolved

nomeata added 3 commits February 9, 2019 11:05

Make set_kind check that the kind is not set a second time

d5baf2b

with a different value. A bit like a promise, but more compatible with `typing.ml` running `infer_dec_typdecs` multiple times. Not sure if it is worth the bother, though.

Address nit

da7d6ae

nomeata added 7 commits February 11, 2019 13:56

Merge branch 'master' into joachim/con-ann

779c806

Refactor avoid to take a con_set, not a con_env

a8f87df

Drop con_env argument to Type.avoid (use kind inside)

0986cff

Remove con_env from scope

51fc8f7

In `typing.ml`, we replace it with a `con_set`, so that we can still call `avoid`. In `check_ir.ml` we simply drop the field.

Actually, get rid of Typing.con_set

933c928

it is always the range of `typ_env` anyways. This simplifies a few more functions.

Delete definition of Type.con_env

0838bc8

Make kind a field and type parameter of Con.t

5eb9c57

Write an .mli file for Check_ir

2b6da66

which uncovers some dead code.

nomeata mentioned this pull request Feb 12, 2019

[WIP, don’t merge] IR pass: Simplify loop forms into a single, simple form #146

Closed

Merge remote-tracking branch 'origin/master' into joachim/con-ann

127ef3a

nomeata requested a review from rossberg February 13, 2019 16:18

This was referenced Feb 13, 2019

Shrink IR #144

Closed

Annotate local constructor environments on blocks #56

Closed

rossberg reviewed Feb 14, 2019

View reviewed changes

Address nits

ceff9e8

nomeata commented Feb 20, 2019

View reviewed changes

src/typing.ml Outdated Show resolved Hide resolved

src/type.ml Show resolved Hide resolved

cleanup kind setting

b25efb3

crusso merged commit 3cb052b into master Feb 20, 2019

nomeata deleted the joachim/con-ann branch February 21, 2019 17:57

Conversation

nomeata commented Feb 8, 2019

Uh oh!

rossberg left a comment

Choose a reason for hiding this comment

Uh oh!

rossberg Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

crusso Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

nomeata Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crusso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nomeata commented Feb 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rossberg commented Feb 11, 2019

Uh oh!

nomeata commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nomeata commented Feb 11, 2019

Uh oh!

rossberg commented Feb 13, 2019

Uh oh!

nomeata commented Feb 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rossberg commented Feb 14, 2019

Uh oh!

Uh oh!

Uh oh!

crusso commented Feb 20, 2019

Uh oh!

rossberg commented Feb 20, 2019

Uh oh!

nomeata commented Feb 20, 2019

Uh oh!

matthewhammer commented Feb 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nomeata Feb 9, 2019 •

edited

Loading

nomeata commented Feb 9, 2019 •

edited

Loading

nomeata commented Feb 11, 2019 •

edited

Loading